Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(caldav): Add default reminder to attendees of scheduling messages #48226

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix(caldav): Add default reminder to attendees of scheduling messages #48226

wants to merge 4 commits into from

Conversation

lufer22
Copy link

@lufer22 lufer22 commented Sep 19, 2024

  • Resolves: Calendar #6315

Summary

The attendees of new scheduled events now have the attendee default reminder set.

TODO

  • ...

Checklist

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 19, 2024
View reviewed changes
apps/dav/lib/CalDAV/Schedule/Plugin.php Fixed Show fixed Hide fixed
apps/dav/lib/CalDAV/Schedule/Plugin.php
$aclPlugin = $this->server->getPlugin('acl');

// Local delivery is not available if the ACL plugin is not loaded.
if (!$aclPlugin) {

Check notice

Code scanning / Psalm

DocblockTypeContradiction Note

Operand of type false is always falsy
apps/dav/lib/CalDAV/Schedule/Plugin.php
$aclPlugin = $this->server->getPlugin('acl');

// Local delivery is not available if the ACL plugin is not loaded.
if (!$aclPlugin) {

Check notice

Code scanning / Psalm

DocblockTypeContradiction Note

Docblock-defined type Sabre\DAV\ServerPlugin for $aclPlugin is never falsy
apps/dav/lib/CalDAV/Schedule/Plugin.php

$caldavNS = '{'.self::NS_CALDAV.'}';

$principalUri = $aclPlugin->getPrincipalByUri($iTipMessage->recipient);

Check failure

Code scanning / Psalm

UndefinedMethod Error

Method Sabre\DAV\ServerPlugin::getPrincipalByUri does not exist
apps/dav/lib/CalDAV/Schedule/Plugin.php
//
// Once we support PHP 5.5, this should be wrapped in a try..finally
// block so we can ensure that this privilege gets added again after.
$this->server->removeListener('propFind', [$aclPlugin, 'propFind']);

Check notice

Code scanning / Psalm

InvalidArgument Note

Argument 2 of Sabre\DAV\Server::removeListener expects callable, but list{Sabre\DAV\ServerPlugin, 'propFind'} provided
apps/dav/lib/CalDAV/Schedule/Plugin.php
[$iTipMessage->sender]
);
}
$objectNode->put($newObject->serialize());

Check failure

Code scanning / Psalm

UndefinedInterfaceMethod Error

Method Sabre\DAV\INode::put does not exist
apps/dav/lib/CalDAV/Schedule/Plugin.php
[$iTipMessage->sender]
);
}
$objectNode->put($newObject->serialize());

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method put on possibly null value
apps/dav/lib/CalDAV/Schedule/Plugin.php
* @param ITip\Message $iTipMessage
* @param int $userDefaultReminder
*/
private function createAlarm(ITip\Message $iTipMessage, int $userDefaultReminder) {

Check notice

Code scanning / Psalm

MissingReturnType Note

Method OCA\DAV\CalDAV\Schedule\Plugin::createAlarm does not have a return type, expecting void
apps/dav/lib/CalDAV/Schedule/Plugin.php
$alarm = $iTipMessage->message->createComponent('VALARM');
$alarm->add($iTipMessage->message->createProperty('TRIGGER', '-' . $this->secondsToIso8601Duration(abs($userDefaultReminder)), ['RELATED' => 'START']));
$alarm->add($iTipMessage->message->createProperty('ACTION', 'DISPLAY'));
$iTipMessage->message->VEVENT->add($alarm);

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method add on possibly null value
apps/dav/lib/CalDAV/Schedule/Plugin.php
$alarm = $iTipMessage->message->createComponent('VALARM');
$alarm->add($iTipMessage->message->createProperty('TRIGGER', '-' . $this->secondsToIso8601Duration(abs($userDefaultReminder)), ['RELATED' => 'START']));
$alarm->add($iTipMessage->message->createProperty('ACTION', 'DISPLAY'));
$iTipMessage->message->VEVENT->add($alarm);

Check failure

Code scanning / Psalm

InvalidArgument Error

Argument 1 of Sabre\VObject\Property::add expects string, but Sabre\VObject\Component provided
github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 24, 2024
View reviewed changes
apps/dav/lib/CalDAV/Schedule/Plugin.php
$userDefaultReminder = $this->config->getUserValue($user->getUID(), 'calendar', 'defaultReminder', 'none');
// If the user hasn't changed the default reminder, it will use the global one
if ($userDefaultReminder === 'none') {
$userDefaultReminder = $this->config->getAppValue('calendar', 'defaultReminder', 'none');

Check notice

Code scanning / Psalm

DeprecatedMethod Note

The method OCP\IConfig::getAppValue has been marked as deprecated
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Oct 4, 2024

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

@ChristophWurst ChristophWurst added the 3. to review Waiting for reviews label Oct 4, 2024
@ChristophWurst ChristophWurst requested review from SebastianKrupinski and removed request for ChristophWurst October 4, 2024 05:21
@miaulalala
Copy link
Contributor

Nice one, I will take a look soon!

@SebastianKrupinski
Copy link
Contributor

Morning @lufer22

Thanks for you effort on this! The code looks good but I do see a logic issue.

  • Firstly since you are completely overriding the parent class code I would keep the parent code in the same function name (scheduleLocalDelivery), instead of a new function (scheduleLocalDeliveryHandler), this is minor and not a big deal.

  • The bigger problem I see is that you are searching the users by email address. The problem here is, that email address in NC are NOT unique. So you can have multiple users with the same email address. So the logic would break if two user accounts have the same address, and the code would use the wrong defaults.

I will see if there is another option to this, this weekend when I have more time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miaulalala miaulalala Awaiting requested review from miaulalala

@tcitworld tcitworld Awaiting requested review from tcitworld tcitworld is a code owner

@SebastianKrupinski SebastianKrupinski Awaiting requested review from SebastianKrupinski SebastianKrupinski is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
3. to review Waiting for reviews feedback-requested
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@lufer22 @miaulalala @SebastianKrupinski @tcitworld @ChristophWurst